Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YJIT: Invalidate JIT code only for ISEQ_TRACE_EVENTS #6695

Merged
merged 1 commit into from Nov 10, 2022

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Nov 8, 2022

Events that are not part of ISEQ_TRACE_EVENTS don't modify ISEQs to use trace_* insns, so we shouldn't need to invalidate everything.

@k0kubun k0kubun marked this pull request as ready for review November 9, 2022 01:59
Copy link
Contributor

@maximecb maximecb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👌

Could you add a small yjit test as well using tracing events that don't touch ISEQs?

@maximecb maximecb requested a review from XrXr November 9, 2022 15:30
@XrXr
Copy link
Member

XrXr commented Nov 9, 2022

I'm a bit surprised that no tests fail, because we rely on patching to fire c_return when we return to a generated C call while the event is on. Not sure how we're firing the event otherwise. If you look at the codegen, we don't put code for firing the event usually.

EDIT: Ah, ISEQ_TRACE_EVENTS includes C_RETURN now since 48c8df9. It looks like now the logic for the else that comes after the diff is outdated now because it assumes RUBY_EVENT_C_RETURN is not part of ISEQ_TRACE_EVENTS?

@XrXr
Copy link
Member

XrXr commented Nov 9, 2022

I think we might want to change only the if condition and keep the invalidation at the end of the function. Theoretically, when we release the VM lock inside rb_yjit_tracing_invalidate_all(), we could yield to another sleeping ractor. With this patch the lock release comes before the global events update, so there is a window where we could compile new code with outdated event info.

@k0kubun k0kubun force-pushed the yjit-invalidate branch 3 times, most recently from 4180d9c to 377e8fa Compare November 10, 2022 18:57
@k0kubun
Copy link
Member Author

k0kubun commented Nov 10, 2022

Addressed feedback from both of you.

It looks like now the logic for the else that comes after the diff is outdated now because it assumes RUBY_EVENT_C_RETURN is not part of ISEQ_TRACE_EVENTS?

Agreed. I wondered why it existed, and looking at it again, I'm sure we can clean it up. But that involves function removals, so I'll make it a separate PR.

@k0kubun k0kubun requested a review from a team November 10, 2022 19:00
@maximecb maximecb merged commit 2b8191b into ruby:master Nov 10, 2022
@maximecb maximecb deleted the yjit-invalidate branch November 10, 2022 22:12
@k0kubun k0kubun mentioned this pull request Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants